Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OmegaConf.missing_keys to get a set of missing keys #719

Merged
merged 19 commits into from
Dec 17, 2021
Merged

OmegaConf.missing_keys to get a set of missing keys #719

merged 19 commits into from
Dec 17, 2021

Conversation

Ohad31415
Copy link
Contributor

No description provided.

@Jasha10
Copy link
Collaborator

Jasha10 commented May 19, 2021

Closes #720

@omry
Copy link
Owner

omry commented Jun 1, 2021

As I said, this is on hold until we start working on 2.2. In the mean time:

  1. Please add tests (including for list and mixing of lists and dicts).
  2. Please add a news fragment
  3. Please update the documentation.

@Ohad31415 Ohad31415 changed the title added OmegaConf.missing_keys to get a set of missing keys OmegaConf.missing_keys to get a set of missing keys Jun 4, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2021

This pull request introduces 1 alert when merging 49b9d46 into 6c732c1 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@Ohad31415
Copy link
Contributor Author

@omry @Jasha10 I have a gap at the docs session, I've added a segment under docs/source/usage.rst for sphinx and seems like I'm missing something.
Would like to get a hand here 😀

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 4, 2021

From the doctest output:

File "usage.rst", line 791, in default
Failed example:
    OmegaConf.missing_keys({
        "foo": {"bar": "???"},
        "missing": "???",
         "list": ["a", None, "???"]
    })
Expected:
    {'list.2', 'foo.bar', 'missing'}
Got:
    {'missing', 'foo.bar', 'list.2'}

The elements of a set are not guaranteed to be printed in any particular order...
Here is something that should make the tests pass, though it is sacrifices clarity for the reader:

>>> sorted(OmegaConf.missing_keys({
...     "foo": {"bar": "???"},
...     "missing": "???",
...     "list": ["a", None, "???"]
... }))
['foo.bar', 'list.2', 'missing']

tests/test_omegaconf.py Outdated Show resolved Hide resolved
@Ohad31415
Copy link
Contributor Author

From the doctest output:

File "usage.rst", line 791, in default
Failed example:
    OmegaConf.missing_keys({
        "foo": {"bar": "???"},
        "missing": "???",
         "list": ["a", None, "???"]
    })
Expected:
    {'list.2', 'foo.bar', 'missing'}
Got:
    {'missing', 'foo.bar', 'list.2'}

The elements of a set are not guaranteed to be printed in any particular order...
Here is something that should make the tests pass, though it is sacrifices clarity for the reader:

>>> sorted(OmegaConf.missing_keys({
...     "foo": {"bar": "???"},
...     "missing": "???",
...     "list": ["a", None, "???"]
... }))
['foo.bar', 'list.2', 'missing']

Yeah it does sacrifice readability, I wonder if there is a way around it to make it verify the output rather than the output's representation.

@omry
Copy link
Owner

omry commented Jun 6, 2021

Yeah it does sacrifice readability, I wonder if there is a way around it to make it verify the output rather than the output's representation.

assert ret == {'list.2', 'foo.bar', 'missing'}

Comment on lines 799 to 803
if not isinstance(cfg, Container):
try:
cfg = OmegaConf.create(cfg)
except ValidationError:
raise ValueError(f"Could not create a config out of {cfg}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be appropriate to use the _ensure_container function here.

Suggested change
if not isinstance(cfg, Container):
try:
cfg = OmegaConf.create(cfg)
except ValidationError:
raise ValueError(f"Could not create a config out of {cfg}")
cfg = _ensure_container(cfg)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. once you do, you can also change your tests to take plain dict/list instead of a Container to simplfy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omry The tests contain inputs of all types: ListConfig, DictConfig, plain list, dict. I think it doesn't hurt having it so, but if you think it's better with only plain lists/dicts I'll change it 😃

Copy link
Owner

@omry omry Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's test everything with plain dict and list, but keep a single test for DictConfig and a single test for ListConfig (just for the top level element).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 I've simplified it adding two tests at the end one with DictConfig and one with ListConfig.

Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please a news fragment file. This is similar to how it's done for Hydra.
Context here.

Comment on lines 799 to 803
if not isinstance(cfg, Container):
try:
cfg = OmegaConf.create(cfg)
except ValidationError:
raise ValueError(f"Could not create a config out of {cfg}")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. once you do, you can also change your tests to take plain dict/list instead of a Container to simplfy.

@Ohad31415
Copy link
Contributor Author

Please a news fragment file. This is similar to how it's done for Hydra.
Context here.

I've added news/720.feature. Let me know if something is wrong/missing.

news/720.feature Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few docs updates:

docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
Ohad31415 and others added 2 commits June 15, 2021 09:20
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>
Ohad added 2 commits June 18, 2021 13:32
Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks @Ohad31415.

@Jasha10 Jasha10 requested a review from jieru-hu October 20, 2021 20:46
@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 20, 2021

I think we can now revisit this PR since work on OmegaConf 2.2 has started.

@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 26, 2021

Made a merge commit to resolve conflicts with master branch.

@Jasha10 Jasha10 added this to the OmegaConf 2.2 milestone Nov 14, 2021
@Jasha10 Jasha10 merged commit 207207a into omry:master Dec 17, 2021
@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 17, 2021

Merged.
Thanks again @Ohad31415!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants